perf: offload /api/v1/videos page read off the event loop - #1382
Conversation
list_videos_v1 is an async endpoint but called count_videos() and get_videos_summary() directly, so both ran on the event loop thread. get_videos_summary "Pass 2" is not cached: for every item on the page it runs parent_dir.glob(), Path.exists(), open() and json.load(). At the default limit of 50 that is roughly 200 blocking syscalls plus up to 50 JSON parses per request, all of which stall every other coroutine on the loop for the duration. Group both reads into _collect_videos_page() and dispatch them with a single asyncio.to_thread hop. One hop rather than two keeps the count and the page consistent with each other and avoids opening a second cache-refresh window between the two reads. Adds TestListVideosOffloading, which asserts where the work runs rather than only what it returns: thread identity for both calls, event-loop responsiveness while a scan is in flight, and preservation of the offset >= total short circuit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
|
@linear-code @coderabbitai review Three judgement calls in this diff that I'd specifically like challenged rather than rubber-stamped:
Please also take a deliberately skeptical look at the new |
Agent Completion Truth Gate: NOT_APPLICABLEEvidence agrees. Machine-readable verdict{
"details": {},
"reasons": [],
"verdict": "not_applicable"
} |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Changed files ==='
git diff --name-only HEAD^ HEAD
printf '%s\n' '=== Router outline ==='
ast-grep outline src/youtube_extension/backend/api/v1/router.py --items all --type function --match 'collect_videos_page|list_videos_v1' || true
printf '%s\n' '=== Router relevant range ==='
rg -n -C 18 'def _collect_videos_page|async def list_videos_v1|count_videos\(|get_videos_summary\(' src/youtube_extension/backend/api/v1/router.py
printf '%s\n' '=== Test outline ==='
ast-grep outline tests/unit/test_v1_router_extended.py --items all --type class,function --match 'TestListVideosOffloading|test_filesystem_scan_runs_on_a_worker_thread|test_event_loop_stays_responsive_while_scan_is_in_flight|test_offset_beyond_total_skips_the_page_read' || true
printf '%s\n' '=== Test relevant range ==='
rg -n -C 20 'class TestListVideosOffloading|def test_filesystem_scan_runs_on_a_worker_thread|def test_event_loop_stays_responsive_while_scan_is_in_flight|def test_offset_beyond_total_skips_the_page_read' tests/unit/test_v1_router_extended.py
printf '%s\n' '=== Diff ==='
git diff HEAD^ HEAD -- src/youtube_extension/backend/api/v1/router.py tests/unit/test_v1_router_extended.pyLength of output: 4393 🏁 Script executed: #!/bin/bash
set -uo pipefail
printf '%s\n' '=== Test file candidates ==='
fd -i 'test_v1_router_extended.py' . || true
printf '%s\n' '=== Test-class locations ==='
rg -n -C 12 'class TestListVideosOffloading\b' . || true
printf '%s\n' '=== Complete list endpoint ==='
sed -n '911,1015p' src/youtube_extension/backend/api/v1/router.py
printf '%s\n' '=== Committed diff ==='
git diff HEAD^ HEAD -- src/youtube_extension/backend/api/v1/router.py || true
git diff HEAD^ HEAD -- tests || trueLength of output: 10795 🏁 Script executed: #!/bin/bash
set -uo pipefail
printf '%s\n' '=== DataService candidates ==='
fd -i 'data_service.py' src . || true
printf '%s\n' '=== DataService method locations ==='
rg -n -C 20 'def count_videos\b|def get_videos_summary\b|def _get_all_files_cached\b|TTL|cache|Lock|RLock' src || true
printf '%s\n' '=== Router query-parameter constraints ==='
sed -n '1,110p' src/youtube_extension/backend/api/v1/router.py
printf '%s\n' '=== DataService imports and class map ==='
ast-grep outline src --items all --type class,function --match 'DataService|count_videos|get_videos_summary|_get_all_files_cached' || trueLength of output: 201102 🏁 Script executed: #!/bin/bash
set -euo pipefail
file='src/youtube_extension/backend/services/data_service.py'
printf '%s\n' '=== Relevant DataService definitions ==='
rg -n '^(class DataService| def __init__| def count_videos| def get_videos_summary| def _get_all_files_cached| def _get_all_files| def clear)' "$file"
printf '%s\n' '=== DataService constructor and file-cache helpers ==='
sed -n '1,260p' "$file"
printf '%s\n' '=== Video count and summary methods ==='
start=$(rg -n '^ def count_videos\b' "$file" | head -1 | cut -d: -f1)
end=$(rg -n '^ def get_video_detail\b|^ def get_learning_log\b' "$file" | awk -F: -v start="$start" '$1 > start {print $1; exit}')
if [ -n "${start:-}" ] && [ -n "${end:-}" ]; then
sed -n "${start},$((end - 1))p" "$file"
elif [ -n "${start:-}" ]; then
sed -n "${start},$((start + 260))p" "$file"
fi
printf '%s\n' '=== Cache invalidation and mutation references in DataService ==='
rg -n -C 5 '_get_all_files_cached|_all_files|cache.*clear|clear.*cache|invalidate|Lock|RLock' "$file"Length of output: 16863
Two hops provide no benefit here. They add dispatch overhead. They also create a larger interval between The cache-consistency explanation needs one correction. One hop does not provide an atomic snapshot. The duplicated The exception contract for real synchronous The offloading tests are substantive. They do not pass vacuously when the calls are skipped. They fail if the helper runs inline. The event-loop test uses a blocking One test gap remains: all three tests pass with this implementation: total = await asyncio.to_thread(data_service.count_videos)
paginated_videos = await asyncio.to_thread(
data_service.get_videos_summary, limit=limit, offset=offset
)That implementation offloads both methods, so it passes the thread-identity and responsiveness checks. It violates the PR’s stated one-hop requirement and reintroduces the larger inter-read scheduling window. Add a regression assertion that I found no production-breaking error-contract regression in the changed endpoint. You are interacting with an AI system. |
There was a problem hiding this comment.
🟡 Not ready to approve
Concurrent requests can trigger duplicate cache-refresh scans on the unsynchronized singleton service.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Offloads blocking video-list filesystem work from the asyncio event loop.
Changes:
- Adds a synchronous page-collection helper.
- Executes collection through
asyncio.to_thread. - Adds offloading and pagination regression tests.
File summaries
| File | Description |
|---|---|
src/.../api/v1/router.py |
Offloads video page collection. |
tests/unit/test_v1_router_extended.py |
Tests worker-thread execution and loop responsiveness. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| total, paginated_videos = await asyncio.to_thread( | ||
| _collect_videos_page, data_service, limit, offset | ||
| ) |
…p count Follow-up to 360589a addressing CodeRabbit's review of #1382. 1. The helper docstring overclaimed. One `asyncio.to_thread` hop is not an atomic snapshot: `DataService` backs both reads with a TTL cache that holds no lock and shares no snapshot object between them, so the entry can still expire — or be refreshed by another worker — between `count_videos()` and `get_videos_summary()`. Reworded to say the grouped hop *narrows* that window to a single thread hand-off rather than eliminating it. 2. The `offset >= total` predicate was duplicated: once inside `_collect_videos_page` and again in `list_videos_v1`. The helper now returns `(total, page, past_end)` and the endpoint branches on `past_end`, so the bounds check lives in exactly one place. Still one dispatch hop. 3. The three offloading tests did not pin the *number* of hops — they all pass for a two-hop implementation that awaits `to_thread` separately per call. Added `test_page_read_uses_exactly_one_to_thread_hop`, which wraps (rather than replaces) the real `asyncio.to_thread` so the work still runs on a worker thread, and asserts exactly one dispatch of `_collect_videos_page`. Negative control NC-4 confirms the new test is load-bearing and the gap was real: under a two-hop implementation the three original tests still pass and only the new test fails, reporting the two dispatched mocks by name. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai Thanks — all three findings were valid and are addressed in 1. The docstring overclaimed consistency — you were rightI checked Reworded to state that grouping the calls narrows the window to a single thread hand-off rather than eliminating it, and that callers must still treat the count and the page as independently observed values. 2. Duplicated predicate — fixed as you suggested
3. The test gap — this was the load-bearing findingYou were right that all three of my tests pass for a two-hop implementation. I did not want to take that on trust either, so I ran it as an explicit negative control (NC-4): I replaced the body with the two-hop variant you described — That is your prediction reproduced exactly: three green, one red. The new test is the only thing standing between this endpoint and a silent regression to two hops, so it earns its place. Per your note I wrap the real One place I'd push back slightlyYour framing of #3 as "the tests do not pin the number of hops" is correct, but I'd add that the two-hop variant is not merely stylistically worse — it re-opens precisely the window described in #1, and does so per request. So #1 and #3 are the same defect seen from two directions: the docstring was describing a property the tests were not enforcing. Fixing only one of the two would have left the code and its documentation disagreeing. Verification after the fix
|
|
Tip For best results, initiate chat on the files or code changes.
Your clarification is correct. The two-hop implementation is a correctness risk, not only an efficiency concern. It expands the interval between the independently observed The revised wording correctly avoids claiming an atomic cache snapshot. The The negative control and focused test result provide sufficient evidence for this change. 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
Independent review — red-team of the diff on top of CodeRabbit's reviewCodeRabbit ran an assertive review and raised three findings. All three were CodeRabbit's findings and their disposition
Findings 1 and 3 are the same defect seen from two directions: the docstring NC-4 — the control that earns the new test its placeI built the two-hop variant CodeRabbit described (a separate A test that fails on precisely the mutation it was written to catch, and on Verification at the true remote headEverything below was re-run in a detached worktree checked out at
Verbatim RED proof: Note this is a stronger RED than the pre-review version, which failed 2 of 3. Three things I deliberately tried to break1. That the tests assert offloading rather than mere extraction. 2. That the error contract is unchanged. 3. That the 4th suite failure belongs to this branch.
Collection order corroborates it: the sdk test is tree line 33, these tests are What CodeRabbit did not doCodeRabbit does not execute the test suite. Its review is static, so the For the record, its own closing assessment after the fixes: it supported the Gate status
All checks are green and |
Canonical issue
Closes #1379
GET /api/v1/videosis declaredasync def, but its body called two blockingDataServicemethods directly on the event loop. Every request therefore parkedthe whole loop — not just its own coroutine — for the duration of an uncached
filesystem walk.
Outcome
The blocking pair is grouped into one synchronous helper and dispatched to a
worker thread with a single
asyncio.to_threadhop:While the scan is in flight the event loop keeps servicing other requests
instead of stalling. The endpoint's response shape, status codes and pagination
semantics are byte-for-byte unchanged.
Why one
to_threadhop and not twocount_videos()andget_videos_summary()are consecutive reads over the sameTTL cache. Wrapping each in its own
to_threadwould yield to the loop betweenthem and halve nothing — it costs a second thread hand-off per request and
widens the window in which the cache can refresh between the two reads.
To be precise about what this does not buy: one hop is not an atomic
snapshot.
DataServiceholds no lock and exposes no snapshot object sharedbetween the two reads, so the entry can still expire — or be refreshed by
another worker — inside the helper. Grouping the calls narrows that window
to a single thread hand-off rather than eliminating it, and callers must still
treat the count and the page as independently observed values. An earlier
revision of this PR claimed the stronger property; CodeRabbit correctly flagged
it and both the docstring and this section now state the weaker, true one.
Why this issue was not already closed
/api/v1/videosstill blockedreal_video_processorcache-directory scanget_video_detail, not the list endpointThe remaining hot path in
get_videos_summary()— "Pass 2" indata_service.py— is not covered by the TTL cache that_get_all_files_cached()provides. Per returned item it performs
parent_dir.glob(...),.exists(),open()andjson.load(). At the defaultlimit=50that is on the order of200 uncached blocking syscalls plus up to 50 JSON parses, all on the event loop.
Scope
Included
src/youtube_extension/backend/api/v1/router.py— new_collect_videos_page()helper returning
(total, page, past_end);list_videos_v1now awaits it via asingle
asyncio.to_threadand branches onpast_end.tests/unit/test_v1_router_extended.py— newTestListVideosOffloadingregression class, 4 tests.
Excluded
DataServiceitself. Making the underlying scan cheaper (orcaching Pass 2) is a separate, larger piece of work and is deliberately left
out so this change stays reviewable and low-risk.
separately rather than bundled here.
Risk
the
awaitinstead of at the original call site. Both sites sit inside thesame pre-existing
try/exceptinlist_videos_v1, so error handling and the500 response path are unchanged. Verified by the untouched
test_list_videos_errorcase, which still passes.other callers, so reverting restores the previous behaviour exactly with no
data migration or config change.
Design choices worth flagging to a reviewer:
data_serviceis passed as an argument rather than closed over, so thehelper stays a module-level pure function that is trivially unit-testable.
offset >= totalshort-circuit was moved into the helper so theredundant page read is skipped inside the worker thread. Rather than have the
caller re-derive the same comparison, the helper now reports it as a third
return value (
past_end), so the bounds rule lives in exactly one place.CodeRabbit flagged the duplicated check in review; this is the fix.
Verification
All commands run from the repository root with
PYTHONPATH=src .venv/bin/python -m pytest ... -p no:cacheprovider --no-cov.-k "ListVideos or list_videos"main(c44494d8d)ruff checkinvocationNegative controls
A green test that cannot fail proves nothing, so each assertion was inverted
against a deliberately broken build:
git checkout origin/main -- router.py(revert the fix entirely)_collect_videos_page, call it inline (noto_thread)offset >= totalshort-circuit from the helperto_threadper call, semantics unchangedNC-2 and NC-4 are the load-bearing ones.
NC-2 proves the tests assert that the work is offloaded, not merely that a
method was extracted — a suite that only checked "a helper exists" would pass
NC-2 and be worthless.
NC-4 was added in response to CodeRabbit's review, which pointed out that the
original three tests all pass for a two-hop implementation. That was correct.
Running the two-hop variant reproduced the prediction exactly — three green,
one red — which is what earns
test_page_read_uses_exactly_one_to_thread_hopits place:
That test wraps the real
asyncio.to_threadrather than replacing it, sothe work still executes on a worker thread and the endpoint keeps its normal
semantics; it asserts the returned
totaland page payload before it assertson the hop count, so it cannot pass vacuously.
Verbatim NC-1 output:
with the assertion message
AssertionError: event loop only advanced 1 time(s) while the scan was running.After each control the source was restored from a pre-edit copy and
git diff --statconfirmed a byte-identical tree before re-running green.Why the existing tests did not catch this
test_list_videos,test_list_videos_paginationandtest_list_videos_errordrive the endpoint through
TestClientwith a mockedDataService. Because themock returns instantly, a blocking call is indistinguishable from a
non-blocking one — the tests assert response shape, never where the work
runs. All three still pass unmodified.
The new tests close that gap by invoking the endpoint coroutine directly with
asyncio.run()(notTestClient, which runs the app on its own thread andwould make thread-identity assertions meaningless):
test_filesystem_scan_runs_on_a_worker_thread— capturesthreading.get_ident()inside the mock and asserts it differs from theloop's thread.
test_event_loop_stays_responsive_while_scan_is_in_flight— parks the mockon a
threading.Event, then counts how many times the loop can tick. REDgives 1 tick; GREEN gives ≥3.
test_offset_beyond_total_skips_the_page_read— asserts the redundant pageread is still skipped past the end of the collection.
Pre-existing failures
The full-suite run reports 4 failures. None are attributable to this change:
tests/test_code_generator.py(3 tests) — these make livegemini-2.5-flash:generateContentnetwork calls. They fail identically onclean
main.tests/test_sdk_python.py::TestEventRelayClient::test_client_no_api_key_header_absent— root-caused; it is an artefact of the local working directory, not of any
branch. Chain of causation:
src/youtube_extension/backend/main.py:57-61callsload_dotenv(dotenv_path=..., override=False)at import time, so anytest that imports the backend loads the repository-root
.envintoos.environ.checkout but not in a freshly created
git worktree.sdk/python/eventrelay_sdk/client.py:57resolvesapi_key or os.environ.get("EVENTRELAY_API_KEY", ""). The test constructsthe client with
api_key="", so a populated environment variable wins andthe
X-API-Keyheader appears — which is exactly what the test assertsagainst.
Four independent checks confirm this, the last two being decisive:
EVENTRELAY_API_KEYset, on cleanmainEVENTRELAY_API_KEYunset, on cleanmain--deselectedThe first two rows reproduce the failure with zero code from this PR
present, purely by toggling an environment variable. The last row removes this
PR's tests from the run entirely and the failure persists. The diff also adds
no
monkeypatch,patch()ofos.environ,setattr,sys.modulesordependency_overridesusage of any kind.This is a genuine latent defect in the test — it should pin the variable with
monkeypatch.delenv("EVENTRELAY_API_KEY", raising=False)instead of relying onambient state — but fixing it belongs in its own change, not in a perf PR
touching an unrelated router. Filed as follow-up work rather than smuggled in
here.
Lint
lint-pythonin.github/workflows/ci.ymliscontinue-on-error: true, scopesto
src/youtube_extension/backend/andsrc/youtube_extension/main.py, andpasses
--ignore E402,F811,F401,F821,B904,B020,E701,E722. Run with that exactinvocation the changed range reports All checks passed!. (A bare
ruff checksurfaces 26 pre-existing errors elsewhere in the tree that areunrelated to this PR.)
ruff formatis not run by CI. Both edited files do report--checkdiffs, buteach remaining hunk reproduces byte-for-byte on clean
mainin a pristineworktree, so it is pre-existing debt. The one hunk that was introduced by this
PR has been collapsed, so this change adds no new formatting drift.
Production evidence
Not applicable — no production surface changes.
This PR alters only where existing work executes, never what it computes.
There is no schema change, no new dependency, no configuration key, no feature
flag and no change to the HTTP contract: the same JSON body, the same status
codes and the same pagination fields are returned for identical inputs. The
only externally observable difference is that concurrent requests are no longer
serialised behind one another's filesystem scans, which is the intended fix.
Consequently there is no production telemetry to attach beyond the CI evidence
above.
Agent handoff
Sibling offload defects on the same async-endpoint layer are tracked as separate
issues and will land as independent PRs:
get_learning_log_v1,get_video_detail_v1,get_cache_stats_v1(which additionally does a redundantdouble
stat()per file) andget_cached_video_v1. TheTestListVideosOffloadingclass here is the intended template for those.